✨ dependencies: hold a PR on blocked until the PRs it depends on merge, across repos - #17689

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign kellyaa for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

@Danathar this PR is back in your court (stage/072-returned-to-contributor). The hive will not merge it until the head changes.

Review asks:

• the review swarm requested changes

Reply here or push a new head to this branch (same sign-off); CI and the review swarm re-run automatically and the PR returns to the normal path.

Queued in your relay fix queue (1 waiting): fixes are handed out before new work as soon as you have capacity.

Triage of the red checks on f221525:

• contract-tests (hive-open-issue helper tests, exit 2 with no FAIL: line) — PR-local. The three new --blocked-on-pr cases in bin/test_hive_open_issue.sh use --repo "acme/widget", but acme is in the script's own placeholder-owner refusal list (case "$REPO_OWNER_LC" in … acme …) so hive-open-issue.sh exits 2 before writing a request; the test file runs under set -e and the first run_script "blockedonprbot" comment --repo "acme/widget" … line aborts the step. Fix: use the owner the sibling cases already use (hiveorg/widgets, e.g. --blocked-on-pr "hiveorg/gadgets#42"), or export HIVE_REPOS for those cases so the repo is in scope. The malformed case (acme/gadget#) will still exit 2 for the right reason once the repo is accepted.
• build-and-test — downstream of contract-tests (contracts=failure); clears with the fix above.
• NOTICE matches the module graph — runner infrastructure, not this diff: go-licenses could not resolve a module source (go-import lookup timed out) and emitted Source: Unknown. Do not commit the generated NOTICE; it needs a rerun.

Fork PR, so the hive is not pushing to this branch.

🐝 Hive Agent: scanner | Instance: hosted-available-oke-11-placeholder-r05x | SHA: unknown

— hive: agent=scanner backend=copilot model=claude-fable-5.1 copilot=1.0.88

Hive status — updated in place; edits do not notify. Full history is in the Activity Monitor.

Review findings from reviewer-2 (folded here because this head already carries its hive review):

CI triage for the red checks at f221525 — one is this PR's, two are not.

contract-tests (and therefore build-and-test) fail because of the new test cases, not the script change.

• bin/test_hive_open_issue.sh:250 runs run_script "blockedonprbot" comment --repo "acme/widget" 7 … --blocked-on-pr "acme/gadget#42".
• bin/hive-open-issue.sh:352 lists acme among the documentation-placeholder owners, so that call refuses with exit 2 ("--repo 'acme/widget' is a documentation placeholder") before anything is parsed.
• The test file runs under set -euo pipefail (line 8) and run_script (line 62–65) does not tolerate a non-zero exit, so the suite aborts right there — which is why the job log ends at PASS: comment --blocked-by refusal writes no request with exit code 2 and no FAIL: line.
• The same applies to blockedonprsamebot (line 263) and badblockedonprbot (line 278); the last one would also "pass" for the wrong reason, since the placeholder refusal fires before the owner/repo#n regex at hive-open-issue.sh:287.

I verified the script change itself does what the test intends: comment --dry-run --repo hiveorg/widgets 7 --blocked-on-pr "hiveorg/gadget#42" emits "blocked_on_pr": 42, "blocked_on_pr_repo": "hiveorg/gadget", and a bare 41 emits no blocked_on_pr_repo. Switching the three new cases to a non-placeholder owner (the rest of the file uses hiveorg/widgets) should turn contract-tests green.

Not this PR's fault:

• NOTICE matches the module graph — the job log says so itself: go-licenses returned Source: Unknown for gopkg.in/natefinch/lumberjack.v2 and gopkg.in/yaml.v3 after a go-import lookup timeout. The diff touches no go.mod/go.sum. A re-run should clear it.
• Sign-off survives the squash / verify PR contents show CANCELLED from a superseded run; the latest runs of both are green.

Scope note: this comment is CI triage only. I have not reviewed the ~1,000 lines of Go in src/pkg/github/pr_blocked_depends_on.go and the automerge/stage-tracker changes; that needs a proper read once CI is green.

Review findings from adjudicator-2 (folded here because this head already carries its hive review):

Security perspective, read at head f221525.

1. The sweep lifts a hive blocked that another path placed, while its target is still open — src/pkg/github/pr_blocked_depends_on.go:265-269

blockedPRMarkedTargets (:386-404) treats every <!-- hive:blocked-on pr=… --> comment as this sweep's own placement. But two other App-authored paths write the byte-identical marker plus blocked on a pull request without any Depends-on: line in the body:

• hive-open-issue comment --repo o/r <PR> --blocked-on-pr N — the watcher only refuses PRs for --needs-decision (issue_request_watcher.go:767-775); the --blocked-on-pr branch (:741-757, :861-865) posts blockedNoteBody (= BlockedOnMarker, issue_blocked.go:1208-1213) and adds blocked on whatever <number> is.
• RecordVerdictBlocker for a PR item (verdict_blocker.go:128-136), now routed through the shared postBlockedOnPRMarker.

Before this PR, SweepBlockedIssues skipped pull requests (issue_blocked.go:1179), so such a hold stayed until a person acted. After it, reconcileBlockedPR sees blocked with the App as latest labeler, marked = {N}, deps = ∅, and liftOrKeepBlockedPR emits N is no longer listed under Depends-on: for the marker (:265-269) → unblockItem + note (:293-300), on the first pass, with N still open. Every lane then takes the PR again. The file comment ("A PR the hive blocked without any marker … is left alone") does not cover a marker from a different writer. Tests cover "target dropped from the body" but not "marker never came from a Depends-on: line".

Suggested fix: only treat a marker as droppable-by-body when it is this sweep's own comment; otherwise fall through to the target read so the block holds until N settles.

--- a/src/pkg/github/pr_blocked_depends_on.go
+++ b/src/pkg/github/pr_blocked_depends_on.go
@@ func (c *Client) reconcileBlockedPR(
-	marked := blockedPRMarkedTargets(comments)
+	marked, viaDependsOn := blockedPRMarkedTargets(comments)
@@
-		c.liftOrKeepBlockedPR(ctx, repo, pr, labels, deps, marked, fresh, cache, res)
+		c.liftOrKeepBlockedPR(ctx, repo, pr, labels, deps, marked, viaDependsOn, fresh, cache, res)
@@ func (c *Client) liftOrKeepBlockedPR(
-	for _, t := range marked {
-		if !listed[t.key()] {
+	for _, t := range marked {
+		// Only a marker this sweep wrote from a Depends-on line may be
+		// dropped by editing the body; a --blocked-on-pr or verdict
+		// marker holds until its target settles.
+		if !listed[t.key()] && viaDependsOn[t.key()] {
 			clauses = append(clauses, t.ref()+" is no longer listed under `Depends-on:`")
 			continue
 		}
@@ func blockedPRMarkedTargets(
-func blockedPRMarkedTargets(comments []*gh.IssueComment) []blockedPRTarget {
+func blockedPRMarkedTargets(comments []*gh.IssueComment) ([]blockedPRTarget, map[string]bool) {
 	seen := map[string]bool{}
+	viaDependsOn := map[string]bool{}
 	var out []blockedPRTarget
 	for _, cm := range comments {
 		for _, m := range blockedOnMarkerRE.FindAllStringSubmatch(cm.GetBody(), -1) {
 			...
 			t := blockedPRTarget{repo: m[1], number: n}
+			if strings.Contains(cm.GetBody(), "says `Depends-on: ") {
+				viaDependsOn[t.key()] = true
+			}
 			if !seen[t.key()] {

(or carry an explicit <!-- hive:blocked-on-source depends-on --> line in blockedPRDependsOnMarkerComment and key on that — sturdier than prose matching.) Please add a test: App blocked + --blocked-on-pr marker, body without Depends-on:, target open → kept.

2. Low — untrusted body drives unbounded cross-repo reads and a growing process map — pr_blocked_depends_on.go:339-371

ParseDependsOn (depends_on.go:34-53) accepts any owner/repo#N; each distinct ref costs one Issues.Get per pass with the App token (:358), and an unreadable ref adds a key to the package-level blockedPRUnreadable map (:343-347) that is never pruned. A contributor body with hundreds of refs to private or non-existent repos is both a per-pass rate-limit drain and a slow memory leak; the marker/lift notes also confirm open/merged/closed state of any PR the App can read. Capping refs per PR (e.g. first 10) and bounding or pruning the map would close it.

Review findings from reviewer (folded here because this head already carries its hive review):

Style / maintainability (read at head f221525): the sweep mirrors SweepBlockedIssues well and reuses the existing marker, note and gate helpers; nothing blocking. Two low notes with fixes:

1. blockedPRUnreadable is a process-lifetime set that only grows — src/pkg/github/pr_blocked_depends_on.go:87-91, written at :361-367. A key is set on the first failed read and never cleared, so (a) the map grows with every PR×ref that ever failed, and (b) if a target later becomes readable and then unreadable again, the warning is never re-logged. The test reaches into the map directly (pr_blocked_depends_on_test.go:285) rather than through a ForTest hook like NoteBlockedOnSeenForTest. Suggest clearing the key on a successful read so the set tracks currently unreadable pairs:

    func (c *Client) openBlockedPRTargets(...) []blockedPRTarget {
    	var out []blockedPRTarget
    	for _, t := range targets {
    		st := c.blockedPRTargetRead(ctx, t, cache)
   +		key := strings.ToLower(repo) + "#" + strconv.Itoa(number) + "|" + t.key()
    		if st.err != nil {
   -			key := strings.ToLower(repo) + "#" + strconv.Itoa(number) + "|" + t.key()
    			blockedPRUnreadable.Lock()
    			...
    			continue
    		}
   +		blockedPRUnreadable.Lock()
   +		delete(blockedPRUnreadable.m, key)
   +		blockedPRUnreadable.Unlock()
    		if st.isPR && st.open {

2. BlockedOnSeenReason duplicates the locked read in blockedFilterReasonFor — new pr_blocked_depends_on.go:98-105 vs existing issue_blocked.go:788-794 (same lock/lookup/blockedFilterReasonPrefix + " on " shape). A small blockedOnSeenRef(repo, number) string accessor next to noteBlockedOnSeen/forgetBlockedOnSeen would let both call it and keep the map's access surface in one file.

Nit: first := append(append([]blockedPRTarget(nil), open...), fresh...)[0] (pr_blocked_depends_on.go:278) allocates a slice to pick one element; first := fresh[0]; if len(open) > 0 { first = open[0] } reads more directly.

Review findings from adjudicator (folded here because this head already carries its hive review):

Correctness review at head f221525.

The lift rule treats every hive-written blocked-on marker on a PR as a Depends-on placement, so a blocked the watcher parked on a PR is lifted on the first pass while its target is still open.

liftOrKeepBlockedPR (src/pkg/github/pr_blocked_depends_on.go:263-277) lifts the hive's blocked when every marked target is either settled or "no longer listed under Depends-on:" — without reading the target in the not-listed case. blockedPRMarkedTargets (:408) collects any <!-- hive:blocked-on pr=… --> marker, not just this sweep's.

But this sweep is not the only hive writer of that marker + label on a pull request: hive-open-issue comment <number|url> --blocked-on-pr N goes through handleOneIssueRequest, which never checks that req.Number is an issue (src/pkg/github/issue_request_watcher.go:741-760 posts blockedNoteBody with the marker and :862 adds BlockedLabel; the only issueIsPullRequest guard there is on the NeedsDecision branch, :767-775). Before this PR such a PR was simply never swept (listOpenBlockedIssues skips IsPullRequest(), issue_blocked.go:1179), so the block held until a person lifted it. After this PR, on the next pass: label is App-latest, marked=[N], body has no Depends-on: → clause "no longer listed" → unblockItem + note, even though N is open. That inverts the watcher's contract ("the blocked sweep lifts the label when the PR merges or closes", issue_request_watcher.go:166-167). TestSweepBlockedPRDependencies_Lift (pr_blocked_depends_on_test.go:348) only exercises markers this sweep wrote, so it does not catch this.

Suggested fix: only let "dropped from the body" count as settled for markers this sweep wrote; for any other hive marker, read the target and keep the block while it is open.

--- a/src/pkg/github/pr_blocked_depends_on.go
+++ b/src/pkg/github/pr_blocked_depends_on.go
@@ func (c *Client) liftOrKeepBlockedPR(
-	for _, t := range marked {
-		if !listed[t.key()] {
+	for _, t := range marked {
+		// Only a marker this sweep wrote came from a Depends-on line, so only
+		// that one lifts when the line is dropped; another hive writer's
+		// marker (the watcher's --blocked-on-pr) holds until its PR settles.
+		if !listed[t.key()] && t.dependsOn {
 			clauses = append(clauses, t.ref()+" is no longer listed under `Depends-on:`")
 			continue
 		}
@@ type blockedPRTarget struct {
 	repo   string
 	number int
+	// dependsOn: the marker comment was written by this sweep.
+	dependsOn bool
 }
@@ func blockedPRMarkedTargets(comments []*gh.IssueComment) []blockedPRTarget {
-			t := blockedPRTarget{repo: m[1], number: n}
+			t := blockedPRTarget{repo: m[1], number: n, dependsOn: strings.Contains(cm.GetBody(), "says `Depends-on: ")}

(key() ignores the new field, so dedupe is unchanged; a target both this sweep and the watcher marked keeps the first-seen flag — if you want "any Depends-on marker wins", OR the flag when seen[t.key()].) A test with a blockedNoteBody-style marker, no Depends-on: line and an open target asserting the label is kept would pin it.

Advisory (not blocking): listOpenPRsForBlockedSweep caps at blockedPRSweepMaxPages = 5 (:47, 500 open PRs); a hive-blocked PR past that page is neither re-marked nor lifted and nothing logs the truncation.

Review follow-ups (below review.backlog.min_severity: P2)

Non-blocking findings below the backlog filing line are listed here instead of as issues, one comment per PR, updated in place. Tick them off as they are fixed; set review.backlog.min_severity: P3 to file every finding.

• P3 low correctness — Open-PR listing capped at 5 pages silently skips hive-blocked PRs beyond 500 (src/pkg/github/pr_blocked_depends_on.go:47): listOpenPRsForBlockedSweep stops after blockedPRSweepMaxPages=5 (500 open PRs) with no log; a PR this sweep blocked that falls past that page is neither re-marked nor lifted. Advisory; large repos on…
• P3 low intent-alignment — Depends-on PR sweep has no per-pass lookup budget, unlike the issue sweep (src/pkg/github/pr_blocked_depends_on.go:383): SweepBlockedPRDependencies (pr_blocked_depends_on.go:117) lists every open PR in every configured repo (up to 5 pages each) and, for each PR with a Depends-on line, issues one Issues.Get per distinct…

merged by Hive trusted-author auto-merge: author @Danathar holds role owner and GitHub permission maintain on repo; CI green at 93948aa.